Repository navigation
mcpx: thin provider layer core — internal/model, model.* settings, intent routing (#1925 slice 1) - #2592
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 53 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d33249c30a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Six findings from the Codex review of d33249c, each fixed in the PR: - P1 routes_consumer: the legacy prov==nil guard no longer gates the model layer; a model.provider-only configuration reaches generate. - P1 consumer/model: auto no longer prefers sampling over a configured provider -- broker presence is not a live sampling path; sampling is auto's fallback and first choice only when named explicitly. - P2 consumer: a model-layer resolution error returns errNoModel instead of falling through to the legacy repair provider. - P2 routes_consumer: handleIntent passes only the explicit request model; repair.model no longer overrides model.default in the layer (the legacy path applies repair.model itself for its label). - P2 consumer: brokerSampler bounds the sampling wait with a context deadline from model.timeout, not only the question TTL that Broker.Get renews. - P2 consumer: the intent session is propagated into the sampling question so Pending session filtering can find it. Tests: model_layer_test.go (resolution-error no-fallback, layer-alone answers, broker session+timeout); TestResolveAuto updated for the new precedence. go test ./internal/model/ ./internal/daemon/ passes on the laptop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cb37afc5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…2592) Round on 6cb37af, all four in the intent/model path: - Explicit provider wins: generate() now takes the request's explicit provider and consults the daemon model layer only when none was supplied -- a request naming provider=claude no longer has its model id sent to the configured model.provider endpoint. - Repair follows generation: handleIntent repairs through the backend that generated the script. A model-layer script is repaired via modelRepairProvider (the layer's backend under the layer's budget); a layer that no longer resolves means no repair, never an unrelated auto-discovered legacy provider. - Budget clamp: modelMaxTokens treats a nonpositive per-call limit as unset, so prompt.maxTokens=-1 can no longer bypass model.budget by being omitted from the request. - Token-limit field: the OpenAI backend sends max_completion_tokens to o-series and GPT-5 models, which reject max_tokens; other models and endpoints keep max_tokens. Tests: TestGenerateExplicitProviderSkipsModelLayer, TestModelMaxTokens, TestModelRepairProviderCarriesLayerBounds, TestTokenLimitParam; the existing chat-completion test now asserts the gpt-5 field. go test ./internal/model/ ./internal/daemon/ pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79aea580d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sample lifecycle) P2 openai.go: Generate no longer sends the invented model id "default" -- Resolve threads model.default into the provider as its only fallback, and an unnamed request with no configured default is ErrUnavailable (callers fall back deterministically). model.default settings text stops promising a backend default. P2 consumer.go: brokerSampler publishes sample.opened before Await and sample.answered after it, mirroring the upstream sampling path, so sample.opened hook answerers can answer model.provider=sampling requests. Tests: no-model sends nothing, configured default reaches the endpoint, broker sampler publishes the lifecycle in order.
…tent routing (#1925 slice 1) Queue task 3215. Approach C from #1925: a Provider interface mcpx owns (Generate; provider/model id namespace), an OpenAI-compatible configured-provider backend honouring model.baseURL/model.apiKeyEnv, the broker sampling path exposed as a backend, model.* settings through the one registry with daemon default model.provider=none (no behaviour change when unconfigured), /v1/intent generation routed through the layer with deterministic fallback, and the docs/consumer.md provider sentence reversed in the same change per SEP-2577. Out of slice: profiles/sets, plugin backend (#47), answerLocally, proxy, script-side generate.
Six findings from the Codex review of d33249c, each fixed in the PR: - P1 routes_consumer: the legacy prov==nil guard no longer gates the model layer; a model.provider-only configuration reaches generate. - P1 consumer/model: auto no longer prefers sampling over a configured provider -- broker presence is not a live sampling path; sampling is auto's fallback and first choice only when named explicitly. - P2 consumer: a model-layer resolution error returns errNoModel instead of falling through to the legacy repair provider. - P2 routes_consumer: handleIntent passes only the explicit request model; repair.model no longer overrides model.default in the layer (the legacy path applies repair.model itself for its label). - P2 consumer: brokerSampler bounds the sampling wait with a context deadline from model.timeout, not only the question TTL that Broker.Get renews. - P2 consumer: the intent session is propagated into the sampling question so Pending session filtering can find it. Tests: model_layer_test.go (resolution-error no-fallback, layer-alone answers, broker session+timeout); TestResolveAuto updated for the new precedence. go test ./internal/model/ ./internal/daemon/ passes on the laptop.
…2592) Round on 6cb37af, all four in the intent/model path: - Explicit provider wins: generate() now takes the request's explicit provider and consults the daemon model layer only when none was supplied -- a request naming provider=claude no longer has its model id sent to the configured model.provider endpoint. - Repair follows generation: handleIntent repairs through the backend that generated the script. A model-layer script is repaired via modelRepairProvider (the layer's backend under the layer's budget); a layer that no longer resolves means no repair, never an unrelated auto-discovered legacy provider. - Budget clamp: modelMaxTokens treats a nonpositive per-call limit as unset, so prompt.maxTokens=-1 can no longer bypass model.budget by being omitted from the request. - Token-limit field: the OpenAI backend sends max_completion_tokens to o-series and GPT-5 models, which reject max_tokens; other models and endpoints keep max_tokens. Tests: TestGenerateExplicitProviderSkipsModelLayer, TestModelMaxTokens, TestModelRepairProviderCarriesLayerBounds, TestTokenLimitParam; the existing chat-completion test now asserts the gpt-5 field. go test ./internal/model/ ./internal/daemon/ pass.
…sample lifecycle) P2 openai.go: Generate no longer sends the invented model id "default" -- Resolve threads model.default into the provider as its only fallback, and an unnamed request with no configured default is ErrUnavailable (callers fall back deterministically). model.default settings text stops promising a backend default. P2 consumer.go: brokerSampler publishes sample.opened before Await and sample.answered after it, mirroring the upstream sampling path, so sample.opened hook answerers can answer model.provider=sampling requests. Tests: no-model sends nothing, configured default reaches the endpoint, broker sampler publishes the lifecycle in order.
56e0dcb to
a9ef359
Compare
|
🤖 Agent note · parked
|
Slice 1 of #1925 (Approach C): the thin provider layer core. Queue task 3215.
What lands
pkgs/mcpx/internal/model(new package) — one small interface mcpx owns:Providerinterface:Name()+Generate(ctx, Request) (Response, error);Requestcarries messages, maxTokens, stop sequences and an optional JSON schema; model ids use theprovider/modelnamespace (SplitModelID).OpenAIProvider): one client, configurable base URL, so OpenAI / a company gateway / LiteLLM / Ollama / LM Studio all work through it. Honoursmodel.baseURLand the key named bymodel.apiKeyEnv; passes a request schema through asresponse_format: json_schema.SamplingProvider): the pre-existing broker sampling path exposed as a backend over an injectedSampleFunc, so sampling survives as one backend among several instead of the only path.Resolve(Config):none(default) resolves to no provider and no error — configuring nothing changes nothing.autoprefers a live sampling path, then a configured provider (only when the user actually configured something: a setapiKeyEnvvariable or an explicit base URL — it never scans the environment for known key names), then fails cleanly.pluginis named in the enum and resolves to unavailable with the reason: that backend is a later slice and depends on build(deps): bump actions/checkout from 3 to 4 #47.model.*settings through the one registry (internal/settings), no new endpoints — config key,MCPX_*env, CLI flag,/v1/settings, MCP tool, per theprompt.*precedent:model.provider(auto|sampling|plugin|openai|anthropic|google|none, daemon defaultnone, matchingprompt.sample: never),model.baseURL,model.apiKeyEnv(opt-in: name the env var; the key itself is never a setting value),model.default,model.budget(hard maxTokens ceiling on the layer; 0 = no layer ceiling),model.timeout.docs/configuration.mdinventory regenerated.Routing:
/v1/intent's generation caller (Server.generate) goes through the layer whenmodel.provideris configured: same catalog slice and system prompt, oneGeneratecall,model.budget/model.defaultapplied, and the daemon wires the sampling backend to the elicitation broker (elicit.SampleAsk+Await, bounded bymodel.timeout). A provider error lands exactly where the scope guard requires: the route returns its deterministic result — the ranked recipes — with the note that no model answered. Withmodel.provider=none(default) the branch is skipped entirely and the existingrepair.*provider path is untouched, including script repair.Docs:
docs/consumer.md's "a configured provider … was not built and should not be" passage is rewritten in the same change, per the issue: SEP-2577 names direct provider APIs as the migration from deprecated Sampling, so the sentence now records the reversal, the layer's shape, and the scope guard that survives it.Deliberately out of this slice (stay on #1925)
model.profiles/model.setsfallback chains, the opencode-plugin backend (#47),sampling.answerLocally, mcpx-as-an-OpenAI-proxy, script-sidemcpx.model.generate, native anthropic/google clients (both are served here through their OpenAI-compatible endpoints).Tests
go test ./...inpkgs/mcpx— newinternal/modeltests cover the chat-completions request shape (provider prefix stripped, key header, schema passthrough), HTTP error surfacing, the sampling adapter, and the Resolve matrix (none/auto/plugin/sampling-without-path). Settings and daemon suites pass, including the regenerated-docs inventory test and the settings-consumed scan (everymodel.*setting is read via an accessor inconsumerPolicyFrom).Need help on this PR? Tag
@codesmith-botwith what you need.